frontend/app/block/blockframe-header.tsx (1)

86-91: Minor: every block header re-renders on any rename start/stop.

renamingBlockIdAtom is a single global atom, so every mounted HeaderTextElems (one per block) re-subscribes and re-renders whenever any block enters/exits rename mode. For typical layouts this is negligible, but a derived per-block atom keeps the subscription scoped:

♻️ Optional refactor

// in blockrenamestate.ts
+export const isRenamingBlockAtomFamily = (blockId: string) =>
+    jotai.atom((get) => get(renamingBlockIdAtom) === blockId);

Then consume useAtomValue(isRenamingBlockAtomFamily(blockId)) instead of comparing the raw id. (Wrap in a memoized atomFamily if you'd rather not re-create the derived atom per render.)

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@frontend/app/block/blockframe-header.tsx` around lines 86 - 91, The header
components subscribe to the global renamingBlockIdAtom so every HeaderTextElems
re-renders on any rename toggle; fix this by introducing a per-block derived
atom (e.g., create an atomFamily or a memoized function like
isRenamingBlockAtom(blockId) that returns jotai.atom(get =>
get(renamingBlockIdAtom) === blockId)) and then have HeaderTextElems consume
useAtomValue of that per-block atom (replace the renamingBlockId === blockId
check with util.useAtomValueSafe(isRenamingBlockAtom(blockId))). If you create
the derived atom inside a factory, memoize it (or use an atomFamily helper) to
avoid recreating it each render.

frontend/app/view/preview/preview.tsx (2)

143-151: Consider also clearing bidir when switching to a different terminal via linkTerm.

linkTerm only writes preview:followtermid; preview:followterm:bidir is preserved. If a user had bidir enabled on terminal A and then switches to terminal B through this menu, the new link silently inherits bidir — which will immediately push a cd into terminal B on the next directory change. If that's intentional, fine; otherwise consider resetting bidir on link to match the unlink semantics at L143–151.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@frontend/app/view/preview/preview.tsx` around lines 143 - 151, linkTerm
currently only updates "preview:followtermid" and leaves
"preview:followterm:bidir" intact, causing a new link to inherit bidir state;
update the linkTerm implementation (the same area interacting with
model.env.services.object.UpdateObjectMeta / WOS.makeORef and model.blockId) to
also clear or explicitly set "preview:followterm:bidir" (e.g., null/false) when
writing the new followterm id so its behavior matches unlink which clears both
keys; ensure any client-side atoms like model.followTermMenuDataAtom are updated
consistently after the UpdateObjectMeta call.

---

7-28: Move top-level helpers below the import block.

shellEscapePath and sendCdToTerminal are declared between two groups of import statements (lines 7–20 vs 21–28). ES module import hoisting makes this work at runtime, but it breaks the conventional top-of-file import layout and trips most linters/formatters. Group all imports first, then define the helpers — or move them into a small helper module.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@frontend/app/view/preview/preview.tsx` around lines 7 - 28, Move the
top-level helper functions out of the middle of the import section: relocate
shellEscapePath and sendCdToTerminal so they appear after all import statements
(or extract them into a new helper module and import them), ensuring you
preserve their signatures and update any imports/exports if you extract them;
this restores a single contiguous import block and keeps the functions
(shellEscapePath, sendCdToTerminal) in their original file or a small helper
module referenced by preview.tsx.

frontend/app/view/preview/preview-model.tsx (2)

173-176: Extract the dropdown menu data type for readability.

The inline anonymous type on followTermMenuDataAtom is long and is duplicated conceptually in preview.tsx when destructuring menuData. Naming it (e.g. FollowTermMenuData) and exporting it simplifies future consumers and keeps the shape in one place.

♻️ Proposed extraction

+type FollowTermMenuData = {
+    pos: { x: number; y: number };
+    terms: { blockId: string; title: string }[];
+    currentFollowId: string | null;
+    bidir: boolean;
+};
+
 ...
-    followTermMenuDataAtom: PrimitiveAtom<{ pos: { x: number; y: number }; terms: { blockId: string; title: string }[]; currentFollowId: string | null; bidir: boolean } | null>;
+    followTermMenuDataAtom: PrimitiveAtom<FollowTermMenuData | null>;

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@frontend/app/view/preview/preview-model.tsx` around lines 173 - 176, Extract
the long anonymous menu-data shape into a named exported type (e.g.
FollowTermMenuData) and replace the inline type on followTermMenuDataAtom with
that alias so the shape is defined once; update any consumers (like preview.tsx
where menuData is destructured) to import and use FollowTermMenuData instead of
repeating the inline type. Ensure the new type is exported from the module that
defines followTermMenuDataAtom and keep the exact field names/structures (pos,
terms array with blockId/title, currentFollowId, bidir) to maintain
compatibility.

---

520-544: Minor: menu fallback title index is based on the filtered list, not tab order.

The i in .map(..., i) => ... || \Terminal ${i + 1}`is the index *after* filtering out non-terminal blocks and the current preview block. Users with mixed blocks will see "Terminal 1"/"Terminal 2" numbers that don't align with tab position. Givenframe:title` is usually present, this is a cosmetic fallback only — flagging as nitpick.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@frontend/app/view/preview/preview-model.tsx` around lines 520 - 544, The
fallback "Terminal N" index is being taken from the filtered/map iteration index
in showFollowTermMenu, which reflects the post-filtered order rather than the
tab order; to fix, build a termBlockIds array from the original tabData.blockids
in the same order (filtering out this.blockId and non-term blocks using
WOS.getObjectValue and block.meta.view === "term"), then when constructing terms
use that ordered termBlockIds to derive the fallback index (e.g., Terminal
${termBlockIds.indexOf(bid) + 1}) instead of the map's i; update the logic that
sets followTermMenuDataAtom accordingly so titles use the tab-order-based index
while keeping frame:title when present.

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

frontend/app/block/blockframe-header.tsx (1)

158-168: cursor-pointer on a non-clickable badge is misleading.

The badge sets cursor-pointer but has no onClick handler; the tooltip instructs the user to right-click. This mismatch suggests left-click should do something. Either drop cursor-pointer (use default/contextmenu cursor), or wire a onDoubleClick={() => startBlockRename(blockId)} for a natural rename-entry UX that matches the cursor affordance.

♻️ Proposed tweak

-            <div
-                key="frame-title"
-                className="block-frame-text shrink-0 opacity-70 cursor-pointer"
-                title="Right-click header to rename"
-            >
+            <div
+                key="frame-title"
+                className="block-frame-text shrink-0 opacity-70 cursor-pointer"
+                title="Double-click to rename (or right-click header)"
+                onDoubleClick={(e) => {
+                    e.stopPropagation();
+                    startBlockRename(blockId);
+                }}
+            >
                 {frameTitle}
             </div>

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@frontend/app/block/blockframe-header.tsx` around lines 158 - 168, The badge
element rendered when useTermHeader && frameTitle is true shows a misleading
cursor via className "cursor-pointer" but has no left-click behavior; either
remove "cursor-pointer" from headerTextElems' div className or add a
left-click/double-click handler to match the affordance—e.g., wire
onDoubleClick={() => startBlockRename(blockId)} on the same div (referencing
startBlockRename and blockId) so the visual cue matches actual behavior; update
the JSX in blockframe-header.tsx accordingly.

frontend/app/view/preview/preview-model.tsx (1)

524-538: Use .map index instead of indexOf for terminal numbering.

termBlockIds.indexOf(bid) inside .map(bid => …) over the same array is O(n²) and redundant — the map callback already receives the index. Block IDs are unique so the result is identical. Also worth collapsing the two iterations (filter + map) into a single pass if you want, but the indexOf fix alone is the main win.

♻️ Proposed refactor

-        const termBlockIds = blockIds
-            .filter((bid) => {
-                if (bid === this.blockId) return false;
-                const block = WOS.getObjectValue<Block>(WOS.makeORef("block", bid), globalStore.get);
-                return block?.meta?.view === "term";
-            });
-
-        const terms = termBlockIds.map((bid) => {
-            const block = WOS.getObjectValue<Block>(WOS.makeORef("block", bid), globalStore.get);
-            const termIndex = termBlockIds.indexOf(bid) + 1;
-            return {
-                blockId: bid,
-                title: (block?.meta?.["frame:title"] as string) || `Terminal ${termIndex}`,
-            };
-        });
+        const terms: { blockId: string; title: string }[] = [];
+        for (const bid of blockIds) {
+            if (bid === this.blockId) continue;
+            const block = WOS.getObjectValue<Block>(WOS.makeORef("block", bid), globalStore.get);
+            if (block?.meta?.view !== "term") continue;
+            terms.push({
+                blockId: bid,
+                title: (block?.meta?.["frame:title"] as string) || `Terminal ${terms.length + 1}`,
+            });
+        }

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@frontend/app/view/preview/preview-model.tsx` around lines 524 - 538, The
current code uses termBlockIds.indexOf(bid) inside the map callback which makes
numbering O(n²); change the map to use its index parameter (e.g.,
termBlockIds.map((bid, i) => { const termIndex = i + 1; ... })) to compute
Terminal N, and optionally collapse the filter+map into a single reduce/map over
blockIds while still skipping this.blockId and checking block?.meta?.view ===
"term" (use WOS.getObjectValue(WOS.makeORef("block", bid), globalStore.get) as
before) so numbering is computed from the iteration index instead of indexOf.

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

frontend/app/view/preview/preview-model.tsx (2)

517-517: Match the declared field type instead of any in the cast.

The field on line 176 is declared with a fully-typed shape, but this initializer weakens it with any on every property. Future globalStore.set(followTermMenuDataAtom, …) calls inside this class won't be checked against the intended shape. Either drop the cast (rely on the field type) or mirror the declared type explicitly.

♻️ Proposed refactor

-        this.followTermMenuDataAtom = atom(null) as PrimitiveAtom<{ pos: any; terms: any; currentFollowId: any; bidir: any } | null>;
+        this.followTermMenuDataAtom = atom(null) as PrimitiveAtom<{
+            pos: { x: number; y: number };
+            terms: { blockId: string; title: string }[];
+            currentFollowId: string | null;
+            bidir: boolean;
+        } | null>;

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@frontend/app/view/preview/preview-model.tsx` at line 517, The initializer for
followTermMenuDataAtom uses a cast to PrimitiveAtom<{ pos: any; terms: any;
currentFollowId: any; bidir: any } | null> which weakens type safety; update the
cast to match the declared field shape (use the exact typed shape used on the
field declaration instead of any) or remove the cast entirely and let TypeScript
infer the type from the declared field; ensure subsequent calls like
globalStore.set(followTermMenuDataAtom, ...) will be type-checked against the
correct { pos: ..., terms: ..., currentFollowId: ..., bidir: ... } | null shape.

---

524-538: Fold filter + map into one pass; use the map index.

A couple of small issues in this block:

• Each candidate block is looked up twice via WOS.getObjectValue (once in filter, again in map).
• termBlockIds.indexOf(bid) inside .map makes terminal numbering O(n²); the index is already available as the second argument of the callback.

Combining them is both clearer and avoids the redundant lookups:

♻️ Proposed refactor

-        const termBlockIds = blockIds
-            .filter((bid) => {
-                if (bid === this.blockId) return false;
-                const block = WOS.getObjectValue<Block>(WOS.makeORef("block", bid), globalStore.get);
-                return block?.meta?.view === "term";
-            });
-
-        const terms = termBlockIds.map((bid) => {
-            const block = WOS.getObjectValue<Block>(WOS.makeORef("block", bid), globalStore.get);
-            const termIndex = termBlockIds.indexOf(bid) + 1;
-            return {
-                blockId: bid,
-                title: (block?.meta?.["frame:title"] as string) || `Terminal ${termIndex}`,
-            };
-        });
+        const terms = blockIds
+            .filter((bid) => bid !== this.blockId)
+            .map((bid) => ({
+                bid,
+                block: WOS.getObjectValue<Block>(WOS.makeORef("block", bid), globalStore.get),
+            }))
+            .filter(({ block }) => block?.meta?.view === "term")
+            .map(({ bid, block }, idx) => ({
+                blockId: bid,
+                title: (block?.meta?.["frame:title"] as string) || `Terminal ${idx + 1}`,
+            }));

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@frontend/app/view/preview/preview-model.tsx` around lines 524 - 538, Replace
the two-pass filter+map over blockIds with a single pass that looks up each
block once using WOS.getObjectValue(WOS.makeORef("block", bid),
globalStore.get), skips this.blockId and only collects blocks whose meta.view
=== "term", and uses the map callback's index (second argument) or a local
counter to derive the terminal number instead of calling
termBlockIds.indexOf(bid); produce an array of objects with blockId and title =
block?.meta?.["frame:title"] || `Terminal ${index+1}`.

frontend/app/view/preview/preview.tsx (2)

24-35: ⚠️ Potential issue | 🟠 Major

Route cd through a shell-aware and foreground-safe path before sending raw input.

This still builds a POSIX/readline payload: \x15cd '<path>'\r. It will not quote correctly for PowerShell/cmd.exe and can be sent into foreground apps like vim, ssh, REPLs, etc. Gate this on terminal shell/prompt metadata or use a backend/controller helper that can safely perform shell-aware directory changes.

#!/bin/bash
# Verify whether preview cd dispatch is still POSIX-only and whether PreviewEnv exposes shell metadata.
set -u

rg -nP -C3 'function shellEscapePath|async function sendCdToTerminal|ControllerInputCommand' frontend/app/view/preview/preview.tsx || true
rg -nP -C3 'shell:type|term:localshellpath|GetMetaCommand|getShellType|foreground|prompt' frontend/app/view/preview/previewenv.ts frontend/app/view/preview/preview.tsx || true

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@frontend/app/view/preview/preview.tsx` around lines 24 - 35, The current
sendCdToTerminal builds and injects a POSIX/readline payload via shellEscapePath
which is unsafe for non-POSIX shells and can be injected into foreground
programs; update sendCdToTerminal to first consult PreviewEnv for
shell/prompt/foreground metadata (PreviewEnv) and, if available, branch by shell
type: for bash/zsh use a safely quoted POSIX cd, for PowerShell use a
Set-Location -LiteralPath-safe call, for cmd.exe use cd /d with proper quoting;
if the terminal is in a foreground app or shell metadata is absent, instead call
a backend/controller helper RPC (e.g. ControllerChangeDirectory or a new
ControllerSafeCd) via env.rpc to perform the directory change server-side rather
than sending raw input with ControllerInputCommand/TabRpcClient; keep
shellEscapePath only for POSIX branch and add a safe literal-quoting function
per-shell or delegate quoting to the backend.

---

309-325: ⚠️ Potential issue | 🟡 Minor

Arm bidirectional suppression only when preview navigation will actually change.

model.goHistory(followTermCwd) can no-op for the current path, but Line 311 still leaves suppressBidirRef.current = true. The next genuine preview navigation then gets suppressed and does not cd the terminal.

🛠️ Proposed guard

     useEffect(() => {
         if (!followTermId || !followTermCwd) return;
+        const curPath = globalStore.get(model.metaFilePath);
+        if (curPath === followTermCwd) return;
         suppressBidirRef.current = true;
         fireAndForget(() => model.goHistory(followTermCwd));
     }, [followTermCwd, followTermId, model]);

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@frontend/app/view/preview/preview.tsx` around lines 309 - 325, The
suppression flag is being set unconditionally before calling model.goHistory, so
a no-op navigation still arms suppression and blocks the next real navigation;
change the first useEffect so you only set suppressBidirRef.current = true when
the preview navigation will actually change the path: either compare
followTermCwd to the model's current cwd (e.g. model.cwd or model.getCwd()) and
only set the flag if they differ, or call model.goHistory(followTermCwd) and set
suppressBidirRef.current = true only if that call returns/indicates a real
change; keep the rest of the effect (fireAndForget) the same and reference
suppressBidirRef, model.goHistory, followTermCwd, and followTermId when making
the change.

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes